Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix quantal stdp test for numpy #681

Merged
merged 2 commits into from
Mar 30, 2017

Conversation

mschmidt87
Copy link

@mschmidt87 mschmidt87 commented Mar 15, 2017

This pull request addresses #680 and contains the following changes:

  • Convert the float variables in the test to integers when they are used as indices to numpy arrays. I decided to do this rather then defining them as integers in the first place because a) defining times as floats seems plausible to me and b) the conversion of t_tot had been implemented in this test at a different location earlier.
  • Remove two parameter dictionaries that were not used in the script.
  • Make travis download the latest release of numpy via pip.

@mschmidt87 mschmidt87 force-pushed the fix_quantal_stdp_test_for_numpy branch from 1e3cf60 to 84b1252 Compare March 15, 2017 03:47
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mschmidt87 Please do not change the travis.yml file in this PR. I will comment on this in the issue, but it is a separate issue and requires careful consideration. I also added some small suggestions to the test.

vm.shape = (n_trials, t_tot)
vm_reference.shape = (n_trials, t_tot)
vm.shape = (n_trials, int(t_tot))
vm_reference.shape = (n_trials, int(t_tot))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using int(t_tot) here to dimension the result array is not a good idea, since you implicitly require that the voltmeter records at 1 ms intervals. A cleaner implementation would be (not 100% sure of spelling of assertions)

self.assert_equal(len(vm) % n_trials, 0)
n_steps = int(len(vm) / n_trials)    # int to be absolutely Py3-safe
vm.shape = (n_trials, n_steps)
...

vm.shape = (n_trials, t_tot)
vm_reference.shape = (n_trials, t_tot)
vm.shape = (n_trials, int(t_tot))
vm_reference.shape = (n_trials, int(t_tot))

vm_mean = numpy.array([numpy.mean(vm[:, i])
for i in range(int(t_tot))])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can avoid the list comprehension here, NumPy supports row- and column-wise means.

@heplesser heplesser requested a review from hakonsbm March 16, 2017 08:26
@heplesser heplesser added ZC: PyNEST DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Mar 16, 2017
Maximilian Schmidt added 2 commits March 16, 2017 21:56
Convert float variables to integers when using them as indices to numpy arrays and make reshaping of voltmeter more general
@mschmidt87 mschmidt87 force-pushed the fix_quantal_stdp_test_for_numpy branch from 84b1252 to c4dfd24 Compare March 16, 2017 13:03
@mschmidt87
Copy link
Author

Thank you for your feedback. I removed the commit touching .travis.yml from the PR and addressed your comments. I furthermore removed t_plot from the script because it was only used for computing the error and I didn't see a reason to not take the entire dataset into account here.

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@heplesser heplesser merged commit ad93653 into nest:master Mar 30, 2017
@mschmidt87 mschmidt87 deleted the fix_quantal_stdp_test_for_numpy branch March 6, 2019 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation ZC: PyNEST DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants